-
-
Notifications
You must be signed in to change notification settings - Fork 268
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adjust dependencies/code for doctrine/annotations 2 compatibility #801
Adjust dependencies/code for doctrine/annotations 2 compatibility #801
Conversation
Regarding the cache, there is already an issue about this it seems: #786 |
394ef78
to
cb6e085
Compare
@greg0ire, I found a little more time to work with this further today, and I've gotten this into a state that passes all tests with both annotations v1 and annotations v2. However, when using annotations v2, caching functionality is going to be lost (for now) because the cache objects do not comply with PSR-6. However, I believe this is written in such a way that if we return PSR-6 compliant objects in future, the caching will begin to work -- I'm not sure if #796 can help with that. Obviously, as noted earlier, I don't have a deep understanding of this module, so it's possible I'm overlooking something important... but hopefully passing tests are a good first step. Please let me know if there's more I can do to help! I've taken this PR out of draft mode so that it can be reviewed. |
In reviewing #800, I was reminded that @andremartinez was encountering phpstan failures. I was able to reproduce the problem and found that it was caused by referencing a non-existent (legacy) Blackhole cache adapter in a test. The easiest solution was to simply skip the test if the newer, expected version of the adapter was missing. I'm not seeing any skipped tests when I run the suite in my environment, so I think this is probably safe for now, especially since things are going to be significantly revised by #796 when that work is completed. |
Apologies, I see that my phpstan fix broke the styles. Everything should be corrected now! |
Thanks, @greg0ire! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are warnings for some lines in the Driver Factory, but those doesn't look like breaking changes. Proper unit testing should be added.
Other than that, everything looks good to me
Thanks, @andremartinez! It may make sense to finish the cache related work before addressing the test coverage warnings. Some lines may not be reachable until that's done, and others may become obsolete. Let me know if I can do more to help, in any case! |
Thanks, @TomHAnderson, let me know if I can do anything else to help (either in terms of contributing to #799, or in terms of improving test coverage in a new PR after #796 is merged). My time is going to be pretty limited for the next couple of weeks, but if I plan ahead, I can help out a little down the road. :-) |
In the interest of moving this forward I've deprioritized #799 at this time though I expect to merge it before a release is made. |
This PR is work in progress on addressing #800.
TODO